refactor(compute): make sandbox readiness gateway-owned across all drivers - #2153
Conversation
elezar
added
test:e2e
|
Label |
|
Label |
0767cc7 to
6ea68c6
Compare
761589e to
0c65b0e
Compare
BlockedHead SHA: Gator is blocked because GitHub reports this PR as Next action: @elezar, please update the PR branch to resolve the merge conflict. Gator will re-check the PR after the branch is mergeable again. |
0c65b0e to
fc29302
Compare
elezar
left a comment
elezar
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: PR #2153 is project-valid because it implements the linked, already-scoped readiness issue #1951 and keeps the changes concentrated in compute runtime readiness behavior and documentation.
Head SHA: 38b20f26d0c0cbfe0f4f16f14248afd3b49bbfe5
Review findings:
- One warning remains about HA readiness composition when the reconciling replica is not the supervisor-session owner.
Docs: Architecture docs were updated in architecture/compute-runtimes.md; no Fern docs change is required for this internal runtime contract until the user-facing lifecycle text changes.
I incorporated @elezar's note that a supervisor connection can arrive before backend Ready and implies the workload is running; that explains the session-precedence arm, but the HA demotion path below still needs attention.
Next state: gator:in-review
elezar
added
gator:in-review
…ecord helpers Split apply_sandbox_update_locked into named helpers to make the two distinct paths explicit: create_sandbox_record for first-observation events and update_sandbox_record for subsequent driver snapshots on existing sandboxes. The dispatcher now uses a match on the existing record rather than an early-return guard. No behavior change. Signed-off-by: Evan Lezar <elezar@nvidia.com>
7bbb126 to
4c596c7
Compare
elezar
left a comment
elezar
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: PR #2153 is project-valid because it implements linked issue #1951's already-scoped gateway-owned sandbox readiness refactor, with changes concentrated in compute readiness, Docker driver reporting, tests, and architecture docs.
Head SHA: 4c596c7cc6544f4e694ff7a7f84efe33e78493bd
Review findings:
- Warning: HA readiness composition still treats the previous public
Readyphase as enough evidence of a recorded supervisor session. After a gateway restart or owner crash, the store can still sayReadywhile the current process has no supervisor session. - Warning: Fern docs are missing for the user-visible lifecycle behavior where a backend-ready sandbox without a supervisor session remains
ProvisioningwithReady=False,SupervisorNotConnected.
Docs: architecture/compute-runtimes.md was updated, but docs/ still needs a user-facing update because this changes CLI/API-visible sandbox phase interpretation.
I incorporated @elezar's note that the HA point was discussed with @drew and checked the latest fix(compute): preserve HA readiness across replicas commit. The current implementation preserves readiness for a non-owner replica, but the remaining restart/owner-loss case below still needs attention.
Next state: gator:in-review
4c596c7 to
2697949
Compare
|
🌿 Preview your docs: https://nvidia-preview-pr-2153.docs.buildwithfern.com/openshell |
elezar
left a comment
elezar
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: PR #2153 is project-valid because it implements linked issue #1951's already-scoped gateway-owned sandbox readiness refactor, with changes concentrated in compute readiness, Docker driver reporting, tests, and architecture/user docs.
Head SHA: 2697949683dd461145b56eb932a3d72ad395fe23
Review findings:
- Warning: the HA section still describes the current process-local session behavior as correct even though @elezar noted the HA implementation is intentionally deferred to #1868. The docs should call this a known limitation, or the implementation needs persisted/leased supervisor presence.
- Suggestion: the new Docker-specific default supervisor image helper duplicates the existing core config source of truth and should either be removed or wired in intentionally.
Docs: docs/sandboxes/manage-sandboxes.mdx now covers the user-visible SupervisorNotConnected lifecycle behavior, and the page is already present in Fern navigation. The architecture HA wording still needs the clarification above.
I incorporated @elezar's note that the HA implementation was discussed and deferred to #1868; I checked the latest code and docs against that deferred scope rather than requiring the HA lease implementation in this PR.
Next state: gator:in-review
…ivers Introduce compute_phase_components and apply_readiness_conditions to centralise the gateway's phase composition logic. The public SandboxPhase is now determined by combining the backend phase reported by the driver with supervisor session presence, independent of the driver implementation. Remove SupervisorReadiness from the driver contract. Running containers always report BackendReady; the gateway owns the Ready decision. Rename the dispatcher match to three arms so that status-less events for existing sandboxes are a documented no-op rather than a silent pass-through. Drop backend_ready_no_session and the SupervisorNotConnected condition. The BackendReady driver condition plus the Provisioning phase already communicates that the backend is up but the supervisor has not connected. The redundant condition added noise without new information. Closes #1951 Signed-off-by: Evan Lezar <elezar@nvidia.com>
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
Signed-off-by: Evan Lezar <elezar@nvidia.com>
2697949 to
74396b8
Compare
elezar
left a comment
elezar
left a comment
There was a problem hiding this comment.
gator-agent
Maintainer Approval Needed
Head SHA: 74396b84e087a449250e198bfd6e4cdf723927f4
Validation: PR #2153 is project-valid because it implements linked issue #1951 and keeps the readiness refactor concentrated in gateway compute composition, Docker backend reporting, tests, and architecture/user docs.
Review: The required independent principal-engineer review found no blocking findings for this head. Previous gator feedback is resolved: @elezar clarified that HA supervisor ownership is deferred after discussion with @drew, the architecture doc now documents that known HA limitation and links it to #1868, the Fern sandbox lifecycle docs cover SupervisorNotConnected, and the Docker default supervisor image source of truth is no longer duplicated.
Docs: architecture/compute-runtimes.md and docs/sandboxes/manage-sandboxes.mdx are updated. No Fern navigation change is needed because the user-facing lifecycle text is on an existing docs page.
Checks: Required Branch Checks, Helm Lint, DCO, Rust, Python, Markdown, license, and preview checks are green for this head.
E2E: test:e2e is applied and OpenShell / E2E is green. GPU E2E is not required because test:e2e-gpu is not applied.
Human maintainer approval is now required.
elezar
added
gator:approval-needed
elezar
added
gator:merge-ready
and removed
gator:approval-needed
Monitoring CompleteMonitoring is complete because this PR has merged. Final status: PR #2153 reached I removed the active |
3 participants
Summary
Centralizes the public
SandboxPhase::Readydecision in the gateway. Compute drivers report backend state only; the gateway composes that state with supervisor session presence to produce the public phase. A structural refactor extracts the dispatch helpers first so the behavioral change remains isolated and reviewable.A sandbox is
Readyonly while its supervisor control session is connected to the gateway. A backend-ready sandbox without that session remainsProvisioningand reportsReady=Falsewith reasonSupervisorNotConnected.Related Issue
Closes #1951
HA supervisor-session ownership and routing remain deferred to #1868.
Changes
crates/openshell-server/src/compute/mod.rs: Extractcreate_sandbox_recordandupdate_sandbox_recordfromapply_sandbox_update_locked; addComposedPhasefor gateway-owned readiness composition; promote on supervisor connect and demote on disconnect; exposeSupervisorNotConnectedwhen the backend is ready without a session.crates/openshell-driver-docker/src/lib.rs: Remove the gateway session registry dependency from the Docker driver and report backend readiness only.crates/openshell-driver-docker/src/tests.rs: Update driver assertions to use backend readiness semantics.crates/openshell-server/src/supervisor_session.rs: Remove the obsoleteSupervisorReadinessimplementation and route session lifecycle changes through the compute runtime.architecture/compute-runtimes.md: Document the gateway readiness composition contract, lifecycle precedence, HA limitation, and compute-driver extension boundary.docs/sandboxes/manage-sandboxes.mdx: Document the user-visibleProvisioning,Ready=False, andSupervisorNotConnectedbehavior, including temporary reprovisioning while a supervisor reconnects after a gateway restart.Deviations from Plan
The PR does not infer cross-replica supervisor presence from a previously stored
Readyphase. Correct HA session ownership, liveness, and request routing require a separately persisted or leased presence signal and remain part of #1868.Testing
mise run docs(0 errors; Fern reports 2 warnings only when requested with--warnings)mise run pre-commitmise run pre-commitis currently blocked by an unusedOrderingimport in the untouchedcrates/openshell-sandbox/src/lib.rs:20Checklist